fix: order -NaN below +NaN in Literal comparison - #861
Conversation
CompareFloat returned lhs_is_negative <=> rhs_is_negative for the both-NaN case, so -NaN compared as greater than +NaN. That contradicts the adjacent "-NAN < NAN" comment and the FloatSpecialValuesComparison / DoubleSpecialValuesComparison tests, which assert the total ordering -NaN < -Infinity < ... < +Infinity < +NaN. Swap the operands so a negative sign bit sorts below a positive one. The existing NaN tests only covered same-sign pairs (qNaN vs sNaN), so the mixed-sign case was unexercised; add FloatSignedNaNComparison and DoubleSignedNaNComparison to cover it.
|
cc @wgtmac FYI |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes NaN total-ordering behavior in Literal::operator<=> so that -NaN sorts below +NaN, aligning implementation with the documented and tested ordering and adding missing mixed-sign NaN coverage (Fixes #860).
Changes:
- Corrected NaN sign-bit comparison in
CompareFloatto order-NaN < +NaN. - Added new float/double tests covering mixed-sign NaN comparisons.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/iceberg/test/literal_test.cc | Adds regression tests for signed NaN ordering for float and double. |
| src/iceberg/expression/literal.cc | Fixes NaN ordering logic to sort negative-sign NaNs below positive-sign NaNs. |
| auto neg_nan = Literal::Float(-std::numeric_limits<float>::quiet_NaN()); | ||
| auto pos_nan = Literal::Float(std::numeric_limits<float>::quiet_NaN()); |
| // A negative sign bit sorts below a positive one (-NaN < +NaN), so a | ||
| // negative operand must compare as less. | ||
| return rhs_is_negative <=> lhs_is_negative; |
There was a problem hiding this comment.
Keeping rhs <=> lhs: it's the idiom for reversing the order relation, and the comment above states the intent. The SignedNaNComparison tests pin the direction either way.
There was a problem hiding this comment.
Can we directly use function proposed by https://parquet.apache.org/blog/2026/05/29/taming-floating-point-statistics-in-apache-parquet-ieee-754-total-order-and-nan-counts/ for IEEE754 total order?
pub fn totalOrder(x: f64, y: f64) -> bool {
let mut x_int = x.to_bits() as i64;
let mut y_int = y.to_bits() as i64;
x_int ^= (((x_int >> 63) as u64) >> 1) as i64;
y_int ^= (((y_int >> 63) as u64) >> 1) as i64;
return x_int <= y_int;
}
It is a piece of Rust code for f64 but would be easy to be adapted to f32.
There was a problem hiding this comment.
Thanks for the pointer. There are two separate questions here and I'd rather ask than guess on the second one.
On the mechanics: std::strong_order already implements the IEEE 754 totalOrder predicate on IEC 559 types, so we don't need to port the bit trick. The bit trick is a bitwise totalOrder and the standard requires std::strong_order to be consistent with totalOrder when is_iec559 is true, so they're equivalent by construction. I also sanity-checked ±0, ±Inf and mixed NaN bit patterns locally and they agree. That part is just a spelling choice.
The semantics are what I'd like your call on. totalOrder distinguishes NaNs by bit pattern, so NaNs differing in the signaling bit or payload are no longer equal to each other. That's finer than the spec asks for: the sorting section only requires -NaN < -Infinity < ... < Infinity < NaN, i.e. sign only, and NaNs aren't permitted as lower or upper bounds anyway. And because Literal::operator== is (*this <=> other) == 0, this changes NaN equality for Literal, not just ordering.
Java isn't a usable reference point here. Comparators uses Comparator.naturalOrder() for float/double, i.e. Double.compare, which canonicalizes every NaN through doubleToLongBits. On JDK 17 I get Double.compare(-NaN, +NaN) == 0 and Double.compare(-NaN, -Infinity) == 1: all NaNs equal, sorting above +Infinity. That contradicts the spec's own -NaN < -Infinity ordering even though the spec claims to align with Java, and our FloatSpecialValuesComparison follows the spec rather than Java.
One thing that does argue for (a): it keeps Literal equality consistent with LiteralValueHash, which hashes floats by bit pattern. Under (b) two same-sign NaNs with different payloads compare equal but hash differently, which breaks the unordered_set invariant behind IN/NOT IN literal sets unless we canonicalize NaN in the hash too. The std::vector<Literal> overload of UnboundPredicateImpl::Make doesn't reject NaN, so such a set is constructible today.
So which do you prefer: (a) full totalOrder via std::strong_order, or (b) keep the NaN special case and only fix the sign inversion, which matches the sorting requirement exactly and leaves NaN equality alone? I've pushed (a) so CI covers it. Switching to (b) is just reverting the last two commits and restoring the same-sign NaN equivalence assertions.
std::numeric_limits<T>::quiet_NaN() does not guarantee a sign bit, so build the mixed-sign NaN operands with std::copysign to keep the test deterministic across platforms.
std::strong_order implements the IEEE 754 totalOrder predicate on IEC 559 types, which is exactly the ordering CompareFloat hand-rolled. Replace the manual NaN sign handling with a direct call. This distinguishes NaNs by bit pattern (a signaling NaN sorts below a quiet NaN of the same sign) instead of collapsing same-sign NaNs to equivalent; update the NaN comparison tests accordingly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/iceberg/expression/literal.cc:439
- The PR description/issue discussion describes fixing only the
-NaNvs+NaNordering by swapping sign-bit operands, but the implementation here switches tostd::strong_orderfor all float comparisons, which also changes semantics for NaNs with different payloads/quiet-vs-signaling bits (and thereforeLiteral::operator==). Please confirm this broader behavior change is intended and align the PR description accordingly; also consider clarifying it in the comment to prevent future confusion.
// Template function for floating point comparison following the Iceberg total
// ordering: -NaN < -Infinity < ... < +Infinity < +NaN. std::strong_order
// implements the IEEE 754 totalOrder predicate on IEC 559 types, which matches
// this requirement (and orders -0 below +0).
template <std::floating_point T>
std::strong_ordering CompareFloat(T lhs, T rhs) {
return std::strong_order(lhs, rhs);
src/iceberg/test/literal_test.cc:219
FloatNaNComparisonassumes (1) signaling NaNs are supported and (2) that a signaling NaN sorts below the quiet NaN returned bynumeric_limits.has_signaling_NaNmay be false on some platforms, and even when true the relative totalOrder between the library’s chosen signaling/quiet bit patterns is not guaranteed. This can make the test non-portable/flaky.
// Identical NaN bit patterns are equivalent under the total ordering.
EXPECT_EQ(nan1 <=> nan2, std::partial_ordering::equivalent);
// Total ordering distinguishes NaNs by bit pattern; a signaling NaN sorts
// below a quiet NaN of the same sign.
EXPECT_EQ(signaling_nan <=> nan1, std::partial_ordering::less);
src/iceberg/test/literal_test.cc:284
DoubleNaNComparisonassumes signaling NaNs exist and thatsignaling_NaN()sorts belowquiet_NaN()under the total ordering.has_signaling_NaNcan be false, and the relative ordering of the implementation-chosen NaN payloads isn’t guaranteed, so the test may be non-portable.
// Identical NaN bit patterns are equivalent under the total ordering.
EXPECT_EQ(nan1 <=> nan2, std::partial_ordering::equivalent);
// Total ordering distinguishes NaNs by bit pattern; a signaling NaN sorts
// below a quiet NaN of the same sign.
EXPECT_EQ(signaling_nan <=> nan1, std::partial_ordering::less);
The quiet-vs-signaling NaN bit pattern and its relative total order are platform/library implementation details, not behavior this code guarantees (the Iceberg ordering only distinguishes NaN by sign, covered by FloatSignedNaNComparison/DoubleSignedNaNComparison). Keep only the same-bit-pattern equivalence assertion in FloatNaNComparison and DoubleNaNComparison.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/iceberg/test/literal_test.cc:276
- Same issue as the float version: calling
quiet_NaN()twice doesn’t guarantee identical NaN bit patterns, butstd::strong_orderonly reportsequivalentfor NaNs with identical bits. Build both literals from the samenanvalue to avoid a platform-dependent/flaky assertion.
auto nan1 = Literal::Double(std::numeric_limits<double>::quiet_NaN());
auto nan2 = Literal::Double(std::numeric_limits<double>::quiet_NaN());
// Identical NaN bit patterns are equivalent under the total ordering.
EXPECT_EQ(nan1 <=> nan2, std::partial_ordering::equivalent);
src/iceberg/test/literal_test.cc:212
quiet_NaN()is called twice but the comment asserts identical NaN bit patterns. The standard doesn’t guarantee repeatedquiet_NaN()calls return the same payload, and IEEE totalOrder (viastd::strong_order) only yieldsequivalentfor NaNs with identical bit patterns. To make the test robust, construct both literals from the same NaN value (same bits).
This issue also appears on line 272 of the same file.
auto nan1 = Literal::Float(std::numeric_limits<float>::quiet_NaN());
auto nan2 = Literal::Float(std::numeric_limits<float>::quiet_NaN());
// Identical NaN bit patterns are equivalent under the total ordering.
EXPECT_EQ(nan1 <=> nan2, std::partial_ordering::equivalent);
What
Literal::operator<=>orders a negative NaN as greater than a positive NaN, the opposite of the total ordering documented and tested in this file.CompareFloatreturnslhs_is_negative <=> rhs_is_negativefor the both-NaN case, so-NaN <=> +NaNistrue <=> false=greater. The adjacent comment says "-NAN < NAN", andFloatSpecialValuesComparison/DoubleSpecialValuesComparisonassert-NaN < -Infinity < ... < +Infinity < +NaN, both of which this branch contradicts.Fixes #860.
How
Swap the operands so a negative sign bit sorts below a positive one:
return rhs_is_negative <=> lhs_is_negative;Testing
The existing
FloatNaNComparison/DoubleNaNComparisontests only cover same-sign NaN pairs (qNaN vs sNaN, which are equivalent), so the mixed-sign case was unexercised. AddedFloatSignedNaNComparisonandDoubleSignedNaNComparisonasserting-NaN < +NaNand the reverse. Verified fail-without (the new tests reportgreater/lessswapped) / pass-with. Fullexpression_testpasses (495 tests).